Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add opt-in go-fuzz modules support #271

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

yevgenypats
Copy link
Contributor

Hey Team,

I know a complete solution for go-fuzz modules support with comprehensive tests is under the works by @thepudds . In the meantime, a lot of people are blocked by this feature and I suggest adding this opt-in environment (GOFUZZ111MODULES=on) variable that I documented that it will go away.

cc @mvdan @josharian @dvyukov

can you please review and approve if this is ok?

Cheers,
Yevgeny

josharian added a commit to josharian/go-fuzz that referenced this pull request Oct 3, 2019
@josharian
Copy link
Collaborator

This is fine, I guess, although I'd prefer something like #272.

I'll leave the decision and merge to Dmitry. I'm ok with either of them going in.

@yevgenypats
Copy link
Contributor Author

I'm totally fine with #272 as well.

@thepudds
Copy link
Collaborator

thepudds commented Oct 4, 2019

#274 is now ready as a viable alternative. (It had been waiting on google/oss-fuzz#2878, which is now addressed as of today).

The most relevant code changes in #274 are also fairly small. The primary distinguishing characteristic for #274 compared to the other PRs is that #274 has tests that exercise go-fuzz with modules enabled, disabled, in auto mode, inside GOPATH, outside GOPATH, with v2+ modules, with replace directives, with a vendor directory, ...

Regarding this change here in #271, I suspect as implemented here, it would probably modify the user's go.mod file in the common case when running go-build-fuzz. If the decision is to pursue #271, then at a minimum that should likely be called out in the readme.

@thepudds
Copy link
Collaborator

thepudds commented Oct 4, 2019

One other nit is that if this PR is pursued, similar logic should be implemented in go-fuzz/main.go as well roughly around here.

Otherwise, I think you can get errors as currently implemented in this PR for some modules based on whether or not the user specifies pkg/func arguments.

@mvdan
Copy link
Contributor

mvdan commented Oct 4, 2019

The PR that @thepudds sent includes lots of tests, so I'd feel a lot safer approving and merging that one instead of this one. Essentially, turning on the knob here isn't guaranteed to work well :)

@yevgenypats
Copy link
Contributor Author

hey @mvdan :) I agree that that @thepudds is a better solution as it's also not opt-in but by default with "auto-detect".

In this PR I didn't just turn-on the knob here I did it as an opt-in so only people who are early adopters and waiting for a long time to try go-fuzz will enable this and they are ok with this feature being deleted later on + having some problems and issues.

https://twitter.com/fuzzitdev/status/1169137825142452225
https://www.reddit.com/r/golang/comments/dc98vv/how_to_fuzz_go_code_with_gofuzz_continuously/
https://twitter.com/search?q=go-fuzz%20modules&src=typed_query

moreover I believe we will find even more small bugs even after @thepudds #274 will be merged or even during review, so there is no bug free solution. I believe if we will merge this it will help many people use go-fuzz as soon as possible + will help us find more bugs and issues faster with go modules support for #274 .

Please consider merging this one:)

cc @dvyukov - @josharian said it's up to you:)

Cheers,
Yevgeny

@dvyukov
Copy link
Owner

dvyukov commented Oct 4, 2019

cc @dvyukov - @josharian said it's up to you:)

my verdict: it's up to @josharian :)

@yevgenypats
Copy link
Contributor Author

@josharian the ball is back in your hands - please:)

@yevgenypats
Copy link
Contributor Author

@josharian friendly ping:)

@josharian
Copy link
Collaborator

OK, I think something like this should go in, as a safety valve. (I intend to review and merge @thepudds's work very soon. But this will provide a useful override mechanism if/when it turns out to be needed.) But I think it should work slightly differently:

I think GOFUZZ111MODULE, if set, should override GO111MODULE. So if you set GOFUZZ111MODULE=auto, then we set GO111MODULE=auto. Use os.LookupEnv to determine whether it is set, and if so, use its value, whatever it happens to be, to set GO111MODULE.

And remove all changes to the README. Let's leave this undocumented, at least for the moment.

@yevgenypats yevgenypats force-pushed the go-fuzz-modules-experiment branch 2 times, most recently from 9085030 to 703c4f1 Compare October 7, 2019 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants